Skip to content

[storage/journal/contiguous] avoid need to fsync when crossing blob boundaries#3790

Open
roberto-bayardo wants to merge 12 commits into
mainfrom
no-fsync-crossing-blobs
Open

[storage/journal/contiguous] avoid need to fsync when crossing blob boundaries#3790
roberto-bayardo wants to merge 12 commits into
mainfrom
no-fsync-crossing-blobs

Conversation

@roberto-bayardo
Copy link
Copy Markdown
Collaborator

@roberto-bayardo roberto-bayardo commented May 14, 2026

The need to fsync when crossing blob boundaries can introduce unexpected delays and increased variance in journal.append() performance. It also complicates making append minimally read blocking.

This PR removes append-time blob-boundary fsyncs. Instead, fixed and variable journals track dirty sections and fsync them when durability is explicitly requested via commit() or sync().

For recovery, fixed journals now persist RECOVERY_WATERMARK_KEY as a conservative replay boundary for layered users such as the variable journal. Fixed-journal recovery itself remains length-based: it recovers the longest contiguous prefix from retained blobs and truncates at the first short or missing section.

Variable-journal recovery continues to treat the data journal as the source of truth. It uses the offsets journal watermark as a bounded replay anchor when safe, rejects stale anchors beyond retained data, and truncates newer data when replay encounters a short retained section.

Detailed Summary

  • Track dirty sections in fixed and variable journals so commit() and sync() can fsync all necessary data.
  • Remove fsync-on-blob-boundary behavior from append paths.
  • Fsync dirty sections without holding the journal write lock, while serializing mutators with an explicit operation mutex.
  • Add fixed-journal recovery watermark metadata as a conservative replay boundary advanced by sync().
  • Preserve legacy recovery behavior for journals without a recovery watermark, then install the watermark during init().
  • Lower the recovery watermark before rewind/clear operations to ensure its lower-bound property even in the event of failures.
  • Rebuild variable-journal offsets from the data journal during recovery, using the offsets watermark as a bounded replay anchor.
  • Validate the offsets recovery anchor against recovered bounds and retained data, falling back to the offsets start when the anchor is stale.
  • Sync init-time tail truncations immediately so repaired storage remains durable across restarts.
  • Add commit metrics for fixed journals to match variable journal observability.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 14, 2026

Deploying monorepo with  Cloudflare Pages  Cloudflare Pages

Latest commit: fd84861
Status: ✅  Deploy successful!
Preview URL: https://5f56fcf7.monorepo-eu0.pages.dev
Branch Preview URL: https://no-fsync-crossing-blobs.monorepo-eu0.pages.dev

View logs

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 14, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
commonware-mcp fd84861 May 21 2026, 05:00 PM

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 14, 2026

Benchmark results

Tip

PASSED: No benchmark exceeded the regression threshold.

Benchmark comparison table
Benchmark Baseline (main) Current Delta Threshold Status
qmdb::merkleize/variant=any::unordered::fixed::mmr keys=10000 ch=false sync=false 1.623 ms 1.581 ms -2.59% 10.00% ✅ PASS
qmdb::merkleize/variant=current::ordered::fixed::mmb chunk=256 keys=10000 ch=true sync=false 3.075 ms 3.104 ms +0.95% 10.00% ✅ PASS

Baseline commit(s): edc5cced171c

@roberto-bayardo roberto-bayardo force-pushed the no-fsync-crossing-blobs branch from 7979ff9 to 6693b28 Compare May 14, 2026 18:40
@roberto-bayardo roberto-bayardo requested a review from Copilot May 14, 2026 18:51
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR optimizes the contiguous journal so that append no longer needs to fsync when crossing section/blob boundaries. Instead, dirty sections are tracked and fsynced only on explicit commit() or sync(). A new DURABLE_SIZE_KEY watermark allows recovery to skip the prefix of fully-durable data while still being able to rebuild the suffix from the data journal when needed.

Changes:

  • Replace per-section auto-sync in append/append_many with deferred fsync gated by a dirty_from_section tracker; introduce an op_lock to serialize mutators with commit/sync while preserving concurrent reads.
  • Persist a durable-size watermark (DURABLE_SIZE_KEY) in fixed journal metadata; rework recovery to use the watermark and rebuild only the offsets suffix in the variable journal.
  • Update qmdb test harnesses to call db.commit() (since apply_batch is no longer implicitly durable) and add new recovery/rewind tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
storage/src/journal/contiguous/fixed.rs Adds durable-size metadata, defers fsync to commit/sync, reworks recovery and adds tests.
storage/src/journal/contiguous/variable.rs Tracks dirty data sections, replaces upgradable lock with rwlock+mutex, rebuilds offsets suffix from anchor, adds tests.
storage/src/qmdb/current/sync/tests.rs Adds explicit db.commit() calls in test harnesses now that apply_batch does not auto-sync.
storage/src/qmdb/any/sync/tests.rs Adds explicit db.commit() calls in test harnesses.

Comment thread storage/src/journal/contiguous/fixed.rs
Comment thread storage/src/journal/contiguous/fixed.rs Outdated
Comment thread storage/src/journal/contiguous/variable.rs
Comment thread storage/src/journal/contiguous/variable.rs Outdated
Comment thread storage/src/journal/contiguous/fixed.rs
@roberto-bayardo roberto-bayardo force-pushed the no-fsync-crossing-blobs branch 3 times, most recently from 08458bf to 435903c Compare May 14, 2026 19:47
@roberto-bayardo roberto-bayardo requested a review from Copilot May 14, 2026 19:47
@roberto-bayardo
Copy link
Copy Markdown
Collaborator Author

bugbot run

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 435903c. Configure here.

@roberto-bayardo roberto-bayardo force-pushed the no-fsync-crossing-blobs branch 2 times, most recently from 7c66e57 to 07df9a9 Compare May 15, 2026 17:31
@roberto-bayardo roberto-bayardo requested a review from Copilot May 15, 2026 17:32
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@roberto-bayardo roberto-bayardo force-pushed the no-fsync-crossing-blobs branch 2 times, most recently from cbd56aa to 30efa74 Compare May 16, 2026 19:35
@roberto-bayardo roberto-bayardo marked this pull request as ready for review May 16, 2026 19:57
@roberto-bayardo roberto-bayardo marked this pull request as draft May 16, 2026 19:57
@roberto-bayardo roberto-bayardo force-pushed the no-fsync-crossing-blobs branch 2 times, most recently from eebd417 to ad87998 Compare May 16, 2026 21:31
@roberto-bayardo roberto-bayardo added the breaking-format This PR modifies codec and/or storage formats. label May 16, 2026
@roberto-bayardo roberto-bayardo moved this to In Progress in Tracker May 16, 2026
Comment thread storage/src/journal/contiguous/fixed.rs
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

@roberto-bayardo roberto-bayardo force-pushed the no-fsync-crossing-blobs branch 4 times, most recently from 8e21f13 to 5c643cb Compare May 19, 2026 15:22
@roberto-bayardo roberto-bayardo force-pushed the no-fsync-crossing-blobs branch from 0642f3e to 8db4cad Compare May 21, 2026 15:20
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8db4cad. Configure here.

Comment thread storage/src/journal/contiguous/fixed.rs
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Comment thread storage/src/journal/contiguous/fixed.rs Outdated
Comment thread storage/src/journal/contiguous/fixed.rs
@roberto-bayardo roberto-bayardo force-pushed the no-fsync-crossing-blobs branch from 4fa5307 to f9b5b59 Compare May 21, 2026 15:51
@roberto-bayardo roberto-bayardo force-pushed the no-fsync-crossing-blobs branch from f9b5b59 to 7d4a2d1 Compare May 21, 2026 15:55
@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

❌ Patch coverage is 96.53821% with 53 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.78%. Comparing base (edc5cce) to head (fd84861).

Files with missing lines Patch % Lines
storage/src/journal/contiguous/variable.rs 93.90% 26 Missing and 5 partials ⚠️
storage/src/journal/contiguous/fixed.rs 98.69% 9 Missing and 4 partials ⚠️
storage/src/journal/segmented/variable.rs 10.00% 8 Missing and 1 partial ⚠️
@@            Coverage Diff             @@
##             main    #3790      +/-   ##
==========================================
+ Coverage   95.77%   95.78%   +0.01%     
==========================================
  Files         486      486              
  Lines      200338   201613    +1275     
  Branches     4858     4879      +21     
==========================================
+ Hits       191872   193123    +1251     
- Misses       6834     6856      +22     
- Partials     1632     1634       +2     
Files with missing lines Coverage Δ
storage/src/journal/contiguous/metrics.rs 100.00% <100.00%> (ø)
storage/src/journal/contiguous/mod.rs 48.64% <ø> (ø)
storage/src/journal/segmented/fixed.rs 97.53% <100.00%> (+<0.01%) ⬆️
storage/src/qmdb/store/db.rs 91.01% <ø> (ø)
storage/src/qmdb/sync/engine.rs 91.78% <ø> (ø)
storage/src/queue/shared.rs 90.71% <ø> (ø)
storage/src/queue/storage.rs 99.06% <ø> (ø)
storage/src/journal/segmented/variable.rs 93.66% <10.00%> (-0.27%) ⬇️
storage/src/journal/contiguous/fixed.rs 98.21% <98.69%> (+1.11%) ⬆️
storage/src/journal/contiguous/variable.rs 97.88% <93.90%> (-0.91%) ⬇️

... and 11 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edc5cce...fd84861. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-format This PR modifies codec and/or storage formats.

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants